Render deterministic functions #3266
Conversation
pyro/infer/inspect.py
Outdated
| rv_label = rv.replace( | ||
| "$params", "" | ||
| ) # incase of neural network parameters |
There was a problem hiding this comment.
Can you revert this change, since it's independent of the main goal of this PR? You're welcome to open a separate issue or PR with suggested changes to rendering settings like this one.
There was a problem hiding this comment.
Yes, but as I define rv_label in the if clause, it should also be defined in the else clause. Otherwise, it will be undefined
There was a problem hiding this comment.
You can just revert to using rv, once you revert this chance there's no need for rv_label.
There was a problem hiding this comment.
@r3v1 looks like this change did not get reverted
|
@r3v1 can you also run |
There was a problem hiding this comment.
@r3v1 can you add a unit test like test_get_model_relations from #3259 that exercises your new functionality, and can you also add a quick example at the end of the model rendering tutorial to demonstrate how to use it?
For the example, you can just copy the model that's already defined in the "Rendering the parameters" section and add a couple of new deterministic variables, e.g.:
def model_deterministic(data):
sigma = pyro.param("sigma", torch.tensor([1.]), constraint=constraints.positive)
mu = pyro.param("mu", torch.tensor([0.]))
x = pyro.sample("x", dist.Normal(mu, sigma))
log_y = pyro.sample("y", dist.Normal(x, 1))
y = pyro.deterministic("y", log_y.exp())
with pyro.plate("N", len(data)):
eps_z_loc = pyro.sample("eps_z_loc", dist.Normal(0, 1))
z_loc = pyro.deterministic("z_loc", eps_z_loc + x, event_dim=0)
pyro.sample("z", dist.Normal(z_loc, y), obs=data)
data = torch.ones(10)
pyro.render_model(
model_deterministic,
model_args=(data,),
render_params=True,
render_distributions=True,
render_deterministic=True
)
pyro/infer/inspect.py
Outdated
| rv_label = rv.replace( | ||
| "$params", "" | ||
| ) # incase of neural network parameters |
There was a problem hiding this comment.
@r3v1 looks like this change did not get reverted
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
As suggested earlier, Without it: And with it: Do I push the change? |
There was a problem hiding this comment.
Do I push the change?
Sorry, I think there was a problem with the diff I was looking at on GitHub. I was specifically requesting the removal of the .replace("$params", "") method call, which I now see you actually did already. The most recent commit as of this review produces the second of your two figures, right?
Other than a couple minor comments I left below, I think this is good to go as a first PR assuming tests pass since it retains the current dev behavior of get_dependencies and render_model as the default. We can change that behavior in a followup PR.
The most recent commit 6fbb58c produces the first picture, so I'm pushing the fix along the other suggestions in the next commit. |
|
Hi @eb8680, do you plan to make a new release with this change? |
|
@r3v1 yes, we'll do a minor release sometime soon. In the meantime you can follow the instructions from the README to install the |
Addresses #3134 and pull request #3259.